Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename vocab attribute to bigram_counts #2195

Closed
wants to merge 1 commit into from

Conversation

souravsingh
Copy link
Contributor

Fixes #2189

@@ -772,7 +772,7 @@ def __init__(self, phrases_model):
for bigram, score in phrases_model.export_phrases(corpus, self.delimiter, as_tuples=True):
if bigram in self.phrasegrams:
logger.info('Phraser repeat %s', bigram)
self.phrasegrams[bigram] = (phrases_model.vocab[self.delimiter.join(bigram)], score)
self.phrasegrams[bigram] = (phrases_model.bigram_count[self.delimiter.join(bigram)], score)
Copy link
Contributor

@menshikh-iv menshikh-iv Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, this doesn't work: bigram_count is not defined

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Sep 24, 2018

Hi @souravsingh, please

  • Read issue again (and related mailing list thread), the main goal isn't renaming: the main point here is to reduce memory usage.
  • Don't rename anything (at least for now), this is really non-trivial (as you can see, you breaks all code-CI now)
  • Try to implement improvement suggested by @piskvorky and don't forget about benchmarks (old vs new) and tests (we must be 100% sure that we don't break compatibility).

@menshikh-iv
Copy link
Contributor

@souravsingh is this abandoned? Should I close this?

@menshikh-iv
Copy link
Contributor

btw, another attempt #2208

@souravsingh
Copy link
Contributor Author

@jenishah really wants to work on this. Closing

@souravsingh souravsingh closed this Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants